Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

POC Pipeline resource interface #442

Merged
merged 1 commit into from
Feb 5, 2019
Merged

POC Pipeline resource interface #442

merged 1 commit into from
Feb 5, 2019

Conversation

shashwathi
Copy link
Contributor

why: Trying to unify different types of pipeline resource when processing input/ output resources.

what:

  • Resource interface defines list of container definitions to download
    and list of container definitions to upload as well.
  • Removed git from build source and add the functionality similar to other resources.

Hopefully this should get us a step closer to having a better resource interface

cc @bobcatfish @pivotal-nader-ziada

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jan 27, 2019
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shashwathi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2019
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 27, 2019
@nader-ziada
Copy link
Member

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2019
@nader-ziada
Copy link
Member

I think the resource interface should be a part of a bigger design proposal covering for example the discoverability of external resource types running on the cluster, I suggest a design doc

nader-ziada
nader-ziada reviewed Jan 28, 2019
View reviewed changes
Copy link
Member

@nader-ziada nader-ziada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great PR, some minor comments

pkg/apis/pipeline/v1alpha1/image_resource.go
@@ -19,6 +19,8 @@ package v1alpha1
import (
"fmt"
"strings"

corev1 "k8s.io/api/core/v1"
)

// NewImageResource creates a new ImageResource from a PipelineResource.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be an opportunity to remove the Image resource till its properly implemented

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a private chat with @bobcatfish about this. I am rephrasing @bobcatfish point here. There is value is keeping image resource because tasks can use its attributes. For eg: in templating params.

pkg/apis/pipeline/v1alpha1/git_resource.go
@@ -30,7 +42,8 @@ type GitResource struct {
// Git revision (branch, tag, commit SHA or ref) to clone. See
// https://git-scm.com/docs/gitrevisions#_specifying_revisions for more
// information.
Revision string `json:"revision"`
Revision string `json:"revision"`
TargetPath string
}

// NewGitResource create a new git resource to pass to Knative Build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to support upload for git?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that would be awesome. I don't know if I want to do that feature in this PR.

@nader-ziada
Copy link
Member

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2019
@bobcatfish
Copy link
Collaborator

I like where this is going!! @shashwathi this seems like it plays nicely into #238, if you are interested in writing up a design doc plz feel free to :D But I think that this change can probably go in beforehand, since it's just internal and we can keep iterating on it :D

bobcatfish
bobcatfish reviewed Jan 31, 2019
View reviewed changes
pkg/apis/pipeline/v1alpha1/resource_types.go
@@ -52,6 +53,9 @@ type PipelineResourceInterface interface {
GetType() PipelineResourceType
GetParams() []Param
Replacements() map[string]string
GetDownloadContainerSpec() ([]corev1.Container, error)
GetUploadContainerSpec() ([]corev1.Container, error)
SetDestinationDirectory(string)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like it!!

@shashwathi
Copy link
Contributor Author

@bobcatfish : Yes the plan is to iterate and have solid resource interface.

@shashwathi
Refactor resource interface
dce7b2f 
Try to unify different types of pipeline resource when processing input
/ output resources. As we add more resources the contract should be more
simple.

Removed git from build source and add the functionality similar to other
resources. Resource defines list of container definitions to download
and list of container definitions to upload as well.
@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/cluster_resource.go 61.3% 71.1% 9.8
pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go 92.9% 90.4% -2.5
pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go 93.8% 91.4% -2.3
pkg/reconciler/v1alpha1/taskrun/resources/pod.go 88.5% 83.6% -4.9

@shashwathi
Copy link
Contributor Author

cc @pivotal-nader-ziada @bobcatfish @tejal29

vdemeester
vdemeester reviewed Feb 5, 2019
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM 👼 👍

@nader-ziada
Copy link
Member

great improvement to the interface 👍

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2019
@knative-prow-robot knative-prow-robot merged commit 7a12815 into tektoncd:master Feb 5, 2019
shashwathi pushed a commit to shashwathi/build-pipeline that referenced this pull request Feb 6, 2019
Only copy certain output resource to intermediate storage
a13ef63 
Accidentally in Pr tektoncd#442 I
introduced the bug for processing image resource. Added test this time
to catch this. Hopefully this should not surface again
@shashwathi shashwathi mentioned this pull request Feb 6, 2019
Merged
shashwathi pushed a commit to shashwathi/build-pipeline that referenced this pull request Feb 6, 2019
Only copy certain output resource to intermediate storage
f79cd1d 
Accidentally in Pr tektoncd#442 I
introduced the bug for processing image resource. Added test this time
to catch this. Hopefully this should not surface again
knative-prow-robot pushed a commit that referenced this pull request Feb 6, 2019
@knative-prow-robot
Only copy certain output resource to intermediate storage
0dd80f4 
Accidentally in Pr #442 I
introduced the bug for processing image resource. Added test this time
to catch this. Hopefully this should not surface again
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nader-ziada nader-ziada nader-ziada left review comments

@bobcatfish bobcatfish bobcatfish left review comments

@vdemeester vdemeester vdemeester left review comments

@tejal29 tejal29 Awaiting requested review from tejal29

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@shashwathi @knative-prow-robot @nader-ziada @bobcatfish @knative-metrics-robot @vdemeester @googlebot